Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design doc for thin logical volumes #442

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

sp98
Copy link
Contributor

@sp98 sp98 commented Mar 4, 2022

Signed-off-by: Santosh Pillai sapillai@redhat.com

@sp98 sp98 changed the title [WIP]Design doc for thin logical volumes Design doc for thin logical volumes Mar 4, 2022
@sp98
Copy link
Contributor Author

sp98 commented Mar 4, 2022

@cupnes @toshipp Please take a look at this design PR for thin volumes when you get some time.

@toshipp toshipp added this to Review in progress in Development Mar 7, 2022
@sp98 sp98 force-pushed the thin-lv-design branch 2 times, most recently from 8b1cdc8 to 55f00cf Compare March 8, 2022 05:49
@cupnes
Copy link
Contributor

cupnes commented Mar 8, 2022

@sp98 I will reply tomorrow. Please wait for a while.

docs/proposals/thin-volumes.md Show resolved Hide resolved
docs/proposals/thin-volumes.md Show resolved Hide resolved
docs/proposals/thin-volumes.md Show resolved Hide resolved
docs/proposals/thin-volumes.md Show resolved Hide resolved
docs/proposals/thin-volumes.md Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
@sp98 sp98 force-pushed the thin-lv-design branch 2 times, most recently from 25ff716 to d3ea546 Compare March 14, 2022 07:34
@cupnes
Copy link
Contributor

cupnes commented Mar 15, 2022

@sp98 @leelavg @travisn
It was determined that it would not be a problem to allow thick and thin volumes to cohabitate in 1VG. We are considering the design that can have both thick and thin pools on the same VG. This design that having multiple device class on the same VG can achieve this. However, only one thin pool can be specified in deviceClass.

For example, we could add the following new field:

  • type: thin
  • thinParams:
    • pool
    • spare-gb
    • overprovision-ratio

A new annotation for capacity management is generated for each device-class corresponding to the thin pool existing in the node.

Supplementary figures are provided. If you have any questions, please ask.
thin-volume

@llamerada-jp llamerada-jp mentioned this pull request Mar 15, 2022
@sp98 sp98 force-pushed the thin-lv-design branch 2 times, most recently from 820e825 to 28a1bba Compare March 16, 2022 07:37
@sp98 sp98 requested a review from cupnes March 16, 2022 09:31
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Show resolved Hide resolved
docs/proposals/thin-volumes.md Show resolved Hide resolved
@sp98 sp98 force-pushed the thin-lv-design branch 3 times, most recently from 7e35f68 to 6450833 Compare March 17, 2022 04:16
Comment on lines 116 to 119
uint64 overprovision_bytes = 4; // Space available on thin pool with overprovision.
float data_free = 5; // Data percent free in thin pool.
float metadata_free = 6; // Metadata percent free in thin pool.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general query, can I use data and metadata percent as double in proto so they'll be represented as float64 in go and there'll be very few conversions?

Is this question about gRPC than this proposal? If so, yes. double is mapped to float64.

https://developers.google.com/protocol-buffers/docs/proto3

In addition, I understood there is no ways to know the actual consumed space in bytes. Using percent unit is OK.

docs/proposals/thin-volumes.md Show resolved Hide resolved

- Does topolvm constantly poll the available capacity of the thin pools?

- How to reload lvmd to use updated lvmd.yaml?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK to find the way during implementation.

docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
docs/proposals/thin-volumes.md Outdated Show resolved Hide resolved
@satoru-takeuchi
Copy link
Member

@sp98 @leelavg @nbalacha

Please reflect on the following comments then I approve this (and @llamerada-jp 's approval is also necessary to merge).

https://github.com/topolvm/topolvm/pull/442/files#r833957396
https://github.com/topolvm/topolvm/pull/442/files#r836245479

Please describe the possible values of OverProvisionRatio.

https://github.com/topolvm/topolvm/pull/442/files#r836222309

Please remove "omitempty" from ThinPoolConfig as well as Type.

https://github.com/topolvm/topolvm/pull/442/files#r836234886
https://github.com/topolvm/topolvm/pull/442/files#r828706602

As discussed in slack, it's not necessary to handle default.

https://github.com/topolvm/topolvm/pull/442/files#r828721468

And some additional notes.

  • Does topolvm constantly poll the available capacity of the thin pools?
    • Topolvm does not poll the available capacity as of now. But we can use checkFunc() to do that. It polls every 1 minute and should not be a performance concern

As I said in another comment, it's done by notify event per 10 minutes.

  • How to reload lvmd to use updated lvmd.yaml?

Please remove this item. Marking it as out-of-scope is also OK.

@leelavg
Copy link
Contributor

leelavg commented Mar 28, 2022

@satoru-takeuchi

  • pardon the delay, as @sp98 is on PTO and me not having collaborator access on PR's repo the delay has happened
  • pls give me a day and I'll think about any alternatives
  • and thanks a lot for consolidating the decisions

As discussed in slack, it's not necessary to handle default.

  • if this is the decision I'll refresh the PR that if thin targetted device-class is the default then WatchResponse.free_bytes will not be set
  • I can add a check in validating device class itself however as we (not Topolvm) intend to use only thinpool targets and having a check isn't feasible
  • how about no change to this for now and this'll be removed as part of dropping Ephemeral support?

@llamerada-jp
Copy link
Contributor

@leelavg

About WatchResponse.free_bytes, it looks either of the following would be a suitable option. Costs will not be much different in either case. Which do you better?

pattern-A:
Set provisioned free bytes to WatchResponse.free_bytes when the device class is thin volume. And Set raw free bytes when device class is a thick volume (Same value as before modify).

pattern-B:
Validate so that thin volume cannot be set as default. And it will be removed as you say.

@leelavg
Copy link
Contributor

leelavg commented Mar 29, 2022

@llamerada-jp I went with a slightly modified pattern-a as below from commit thinp: updates to vgservice rpc for thinpools (as of now it's 6e49f8e)

Set provisioned free bytes [...]

  • Set overprovisioned free bytes [...]
		case TypeThick:
			if dc.Default {
				res.FreeBytes = vgFree
			}
// [...]
		case TypeThin:
// [...]
// here opb is the free overprovision bytes
			opb := uint64(math.Ceil(dc.ThinPoolConfig.OverprovisionRatio*float64(tpu.SizeBytes))) - tpu.VirtualBytes
			if dc.Default {
				// TODO (leeavg): remove this after removal of support for inline ephemeral volumes
				res.FreeBytes = opb
			}
// [...]
  • as a gist, we are on same page and this is also finalised IMHO

@llamerada-jp
Copy link
Contributor

as a gist, we are on same page and this is also finalised IMHO

OK. Would you please update the comment about it at the next push?

Signed-off-by: Santosh Pillai <sapillai@redhat.com>
Co-authored-by: Leela Venkaiah G <lgangava@redhat.com>
@leelavg
Copy link
Contributor

leelavg commented Mar 30, 2022

Copy link
Contributor

@cupnes cupnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM

Copy link
Contributor

@llamerada-jp llamerada-jp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@llamerada-jp llamerada-jp merged commit 9b70099 into topolvm:main Apr 4, 2022
Development automation moved this from Review in progress to Done Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants